-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/better find package #1019
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
Prior check will not be correctly interpreted.
@Mizux do you think this PR could be integrate into the code ? |
First, thanks for your contribution ! If I understand correctly, you just split the But in this case the new macro
ref: https://cmake.org/cmake/help/latest/command/find_package.html#basic-signature-and-module-mode Also, one other side topic: I think I'll have to change the CMake behavior, I would like to install deps first then consume the install, than trying to use it as Sorry for the delay, I'm still thinking about it and where I would like to go. |
Using CMAKE_MODULE_PATH & Find<Package>.cmake files simplify how we handle the package provide by CMake (ie: ZLIB & Protobuf).
CMakeLists.txt
Outdated
endif() | ||
if(NOT abseil_DIR AND BUILD_DEPS) | ||
set(abseil_DIR ${CMAKE_CURRENT_SOURCE_DIR}/cmake/external CACHE PATH "abseil dependency path") | ||
list(APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_SOURCE_DIR}/cmake/external/abseil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
abseil-cpp ? (I can also do it in a later commit)
just becaue there is also https://github.com/abseil/abseil-py and maybe we will need it one day too
note: shame on me of not doing it too ;)
@Mizux I rework the way this branch handle its dependencies in 2a003e0 the branch now donwload, build & install its file in the build directory (no more subproject). I have however the following compilation error which I do not understand and I would like your help to see what I could have missed.
Edit: I did remove some patch in order to be the closest to what could be the source install by a user, I think that moving away from those patch (like some amelioration to the |
Hi, I revamped your PR in #1116 (commit a49b914) but with few changes:
Any feedback are welcome, hope you'll better understand what I'm aiming for.... |
I'm closing this PR since we have #1116 |
This PR is a proposal to fix #732 using
find_package
in order to have better granularity for handling dependencies (this is inspired by https://gist.github.com/Som1Lse/6458d28c4bb09163a5389f24fec981db).Thanks to the built-in variable
<package_name>_DIR
variable we can override the default path forfind_package
in order to use :BUILD_DEPS=ON
or already download and provide by the user through a custom<Package_name>Config.cmake
)Allowing to use
find_package
in order to resolve out of tree dependencies allow or-tools to integrate nicely with Yocto, which is not the case today as Yocto since we cannot relay on theBUILD_DEPS
switch in Yocto (it does not allow to download dependency during cmake configure time).